Skip to content

Normalize whitespace and strip stray <br> tags in spreadsheet export (BL-16518)#8074

Open
StephenMcConnel wants to merge 4 commits into
masterfrom
BL-16518-SSExportNotNormalized
Open

Normalize whitespace and strip stray <br> tags in spreadsheet export (BL-16518)#8074
StephenMcConnel wants to merge 4 commits into
masterfrom
BL-16518-SSExportNotNormalized

Conversation

@StephenMcConnel

@StephenMcConnel StephenMcConnel commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What

When exporting a book to a spreadsheet, each bloom-editable's content was written to the cell verbatim (editable.InnerXml), so un-normalized text leaked into the exported spreadsheet: leading/trailing whitespace, stray <br>/<br /> tags, and runs of multiple whitespace characters.

Change

In SpreadsheetExporter.WriteTranslationGroup, after extracting each editable's content (and converting the audio-split marker to |), the content is now normalized before being written to the cell:

  • trim leading/trailing whitespace;
  • remove <br> / <br /> tags (which should not be present in exported text);
  • collapse runs of whitespace to a single space.

SpreadsheetExporterTests expectations were updated to the normalized (single-space) output.

Testing

  • Build clean; Spreadsheet C# unit tests pass (SpreadsheetExporterTests, round-trip, and general spreadsheet tests).

Note: This replaces the earlier "normalize the whole book via BookProcessor.ProcessBook before export" approach (which was reported as being too slow). This version normalizes the exported text directly in the exporter instead.

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16518

Devin review


This change is Reviewable

Description updated by [Claude Opus 4.8] during preflight to reflect the current implementation.

@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR normalizes exported spreadsheet cell content by replacing <br>/<br /> tags with a space, collapsing runs of ordinary whitespace (deliberately excluding non-breaking spaces), and trimming — fixing un-normalized text that leaked into exports verbatim.

  • SpreadsheetExporter.WriteTranslationGroup applies the three normalization steps after the audio-split-marker substitution, with clear inline comments explaining each decision (especially the non-\\s regex rationale).
  • Two existing test assertions are updated to the now-normalized (single-space) form, and two new test classes provide targeted coverage: one for the unit-level normalization behaviours (br → space, multi-space collapse, NBSP preservation) and one regression suite exercising split-TextBox and sentence-mode audio pages where a <br> between sentences must not fuse them.

Important Files Changed

Filename Overview
src/BloomExe/Spreadsheet/SpreadsheetExporter.cs Adds targeted whitespace normalization after content extraction: replaces <br>/<br /> with a space, collapses ordinary whitespace runs with a regex that correctly excludes non-breaking spaces, then trims. Logic is correct and well-commented.
src/BloomTests/Spreadsheet/SpreadsheetAudioTests.cs Adds a focused regression-test class covering split-TextBox and sentence-mode audio pages with <br> + multi-space boundaries; includes sanity-check assertions in OneTimeSetUp to prevent false passes.
src/BloomTests/Spreadsheet/SpreadsheetExporterTests.cs Updates two existing assertions from double-space to single-space output and adds ExportNormalizesContent_SpacesOutLineBreaks_CollapsesWhitespace_PreservesNbsp covering all three normalization behaviours in one shot.

Reviews (5): Last reviewed commit: "Add audio regression tests for line-brea..." | Re-trigger Greptile

@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin up to 1233bb3 — review complete, no Bug or Investigate findings. Devin's summary correctly describes the change (normalize via BookProcessor.ProcessBook before reading the export DOM, gated on IsSaveable, with folder-rename reconciliation). Greptile and CI are also green.

@hatton

hatton commented Jul 17, 2026

Copy link
Copy Markdown
Member

This is going to be REALLY slow...

StephenMcConnel and others added 2 commits July 20, 2026 10:55
…(BL-16518)

When exporting a book to a spreadsheet, each bloom-editable's content was
written to the cell verbatim, so leading/trailing whitespace, stray <br>
tags, and runs of whitespace leaked into the exported spreadsheet. Trim the
content, remove <br>/<br /> tags (which should not be present), and collapse
runs of whitespace to a single space so the exported text is normalized.

Update SpreadsheetExporterTests expectations to the normalized (single-space)
output.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@StephenMcConnel
StephenMcConnel force-pushed the BL-16518-SSExportNotNormalized branch from 1233bb3 to f07c8a0 Compare July 20, 2026 16:56
@StephenMcConnel StephenMcConnel changed the title Normalize book pages before spreadsheet export (BL-16518) Normalize whitespace and strip stray <br> tags in spreadsheet export (BL-16518) Jul 20, 2026
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Good call — the approach here has been changed to address exactly that concern. It no longer runs BookProcessor.ProcessBook on export (no per-page off-screen browser pass). Instead it normalizes the exported text directly in SpreadsheetExporter.WriteTranslationGroup — trim, strip stray <br>/<br />, and collapse whitespace runs — so there's no added slowness. The branch was force-pushed; the PR now reflects this simpler approach (old ProcessBook/reconcile code is gone).

Comment thread src/BloomExe/Spreadsheet/SpreadsheetExporter.cs Outdated
Comment thread src/BloomExe/Spreadsheet/SpreadsheetExporter.cs Outdated
Comment thread src/BloomExe/Spreadsheet/SpreadsheetExporter.cs Outdated
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin on 2026-07-20 11:08 MDT up to commit f07c8a0c.

Devin found 2 current bugs, both in SpreadsheetExporter.cs, posted as review threads and left open for a decision (they change user-facing export behavior):

  • Line breaks deleted, joining adjacent words — :414 (Greptile flagged this too)
  • Non-breaking spaces collapsed to ordinary spaces — :416

One Investigate flag ("diff doesn't match the ProcessBook description") was a false positive once the PR description was rewritten to match the new approach — replied and resolved. All other Devin items (the book-name-rename bug and the SpreadsheetApi.cs reconcile/threading notes) were about the superseded ProcessBook approach and are Resolved/Informational — no action.

CI checks green; Greptile green.

…in spreadsheet export (BL-16518)

Address review findings (Devin, Greptile, and the preflight review) on the new
export normalization:

- Collapse only ordinary (breaking) whitespace instead of \s, so non-breaking
  spaces (U+00A0, U+202F, ...) are preserved rather than silently converted to
  ordinary spaces. These are meaningful, e.g. French punctuation spacing.
- Replace <br>/<br /> with a single space rather than an empty string, so words
  on either side of a soft line break (Shift+Enter) are not run together.

Add a test exercising all three normalization behaviors (line-break spacing,
whitespace collapse, and non-breaking-space preservation).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin on 2026-07-20 11:42 MDT up to commit 81e23ab6. Re-review clean — the two prior bugs (line-break word-join at :414, non-breaking-space collapse at :416) are now Outdated after the fixes in 81e23ab6; both threads were replied to and resolved. No current Bug or Investigate findings.

… export (BL-16518)

Closes the coverage gap identified in review: no test exercised audio-bearing
content that contains a <br> between sentences (or runs of extra whitespace)
through the exporter's normalization. If normalization fused two <br>-separated
sentences (as the earlier <br>->"" code would have), the re-imported text would
fragment into fewer sentences than there are recorded audio segments, dropping
or desyncing the audio.

Adds Spreadsheet_AudioWhitespaceNormalization_Tests covering both split-TextBox
mode (two timed segments, endtimes preserved) and Sentence mode (one file per
sentence), asserting the sentence boundary survives export as a single space and
the segment/endtime/audio-file mapping stays intact.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread src/BloomExe/Spreadsheet/SpreadsheetExporter.cs
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin on 2026-07-20 14:40 MDT up to commit 9630e677 (test-only commit). The three prior bugs are now marked Resolved. One new Investigate flag — soft <br> line breaks in page content now flatten to a space instead of a round-trippable newline — was posted, confirmed as the intended behavior (deliberate per this PR; genuine line breaks use bloom-linebreak spans and are unaffected), and resolved. Remaining items are Informational or about the superseded ProcessBook approach. No open findings.

@StephenMcConnel
StephenMcConnel marked this pull request as ready for review July 20, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants